-
Notifications
You must be signed in to change notification settings - Fork 62
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix deleting a disk from VM #348
Conversation
06681ee
to
1fcf320
Compare
@@ -272,7 +272,6 @@ def vm_reconfigure(vm, options = {}) | |||
# Retrieve the current representation of the virtual machine: | |||
# TODO: no need to retreive vm here, only if memory is updated | |||
vm = vm_service.get | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
seems this space line is required
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
added it back
Removing a disk was not handled correctly Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1666593
1fcf320
to
70f427f
Compare
Checked commit borod108@70f427f with ruby 2.3.3, rubocop 0.52.1, haml-lint 0.20.0, and yamllint 1.10.0 spec/models/manageiq/providers/redhat/infra_manager/ovirt_services/strategies/v4_spec.rb
|
disk_specs.each do |disk_spec| | ||
attachment_service = attachments_service.attachment_service(disk_spec['disk_name']) | ||
disk_spec = disk_spec.with_indifferent_access |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@borod108 just curious, why do you need with_indifferent_access here? Is the 'disk_name'
key not always a string ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When the request comes from api it is, at least sometimes, a symbol (this was one of the things that went wrong here)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Interesting ok thanks
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Fix deleting a disk from VM (cherry picked from commit c4063dd) Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1687846
Hammer backport details:
|
Removing a disk was not handled correctly
Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1666593